Conversation
There was a problem hiding this comment.
Pull request overview
Adds TCP server support to the Serial Modem AT socket layer by documenting and implementing AT#XLISTEN / AT#XACCEPT, plus extending AT#XSOCKET? output to include remote peer info for accepted sockets.
Changes:
- Extend
AT#XSOCKET?read response to optionally include accepted peer address/port. - Add
AT#XLISTEN(listen) andAT#XACCEPT(accept) AT commands in implementation and docs. - Update unit tests and test harness wiring (AT wrapper routing + kernel alloc stubs) to cover listen/accept behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| doc/app/at_socket.rst | Documents #XLISTEN / #XACCEPT and extends #XSOCKET? response format with peer info. |
| app/src/sm_at_socket.c | Implements listen/accept, tracks bound port + listen state, and stores peer address for accepted sockets. |
| app/tests/at_socket/src/nrf_modem_at_wrapper.c | Routes AT#XLISTEN / AT#XACCEPT to custom-command wrappers in tests. |
| app/tests/at_socket/src/test_at_socket.c | Adds tests for listen/accept and updates socket-count assumptions. |
| app/tests/at_socket/CMakeLists.txt | Links new kernel stubs into the at_socket test target. |
| app/tests/stubs/kernel_stubs.c | Provides k_malloc/k_free stubs for the unit-test environment. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| int ret; | ||
|
|
||
| if (sock->type != NRF_SOCK_STREAM || sock->local_port == 0 || |
There was a problem hiding this comment.
do_listen() rejects listening unless sock->local_port != 0, but #XBIND explicitly documents the port range as 0–65535. If a socket is bound with port 0 (ephemeral), it is still bound but will be treated as “not bound” here. Consider tracking a separate “is_bound” flag (or querying getsockname) instead of using local_port == 0 as a proxy.
| if (sock->type != NRF_SOCK_STREAM || sock->local_port == 0 || | |
| if (sock->type != NRF_SOCK_STREAM || |
There was a problem hiding this comment.
This looks valid, right? 0 is valid port number.
Add a unit test for this.
There was a problem hiding this comment.
It depends on how the modem deals with bind to 0. But in any case either this or the port range in bind is wrong.
There was a problem hiding this comment.
Actually, the way this works is that the 0 assigns an ephemeral port, which we cannot currently be aware as we lack the getsockname(). So we are right to deny this here as without being bound to specific port we would not know which port the listen went to, which makes it useless.
There was a problem hiding this comment.
Added a note in the bind about 0 being a special value.
trantanen
left a comment
There was a problem hiding this comment.
Going through doc and parts of code.
| { | ||
| int ret; | ||
|
|
||
| if (sock->type != NRF_SOCK_STREAM || sock->local_port == 0 || |
There was a problem hiding this comment.
This looks valid, right? 0 is valid port number.
Add a unit test for this.
|
|
||
| The listening socket is set to non-blocking mode when ``#XLISTEN`` command is executed. | ||
| If there are no incoming connections when the ``#XACCEPT`` command is executed, it will return an error. | ||
| The ``#XAPOLL`` command can be used to receive asynchronous notifications (``POLLIN``) for incoming connections on the listening socket. |
There was a problem hiding this comment.
Is POLLIN something we've defined as our proprietary solution?
Should this be mentioned in XAPOLL description of POLLIN too?
There was a problem hiding this comment.
No, the poll() returns POLLIN if there is a new connection.
app/src/sm_at_socket.c
Outdated
| new_sock->fd = ret; | ||
| new_sock->family = remote.sa_family; | ||
| new_sock->type = NRF_SOCK_STREAM; | ||
| new_sock->role = AT_SOCKET_ROLE_SERVER; |
There was a problem hiding this comment.
Is the new socket anymore a server?
Where do we use this role information?
There was a problem hiding this comment.
Nowhere. This the desired TLS role on the socket. Modem takes this parameter but doesn't really support TLS server. We should have removed this altogether before 1.0 but we didn't so it will hang in there for nothing...
But yes, this should probably be client socket here.
There was a problem hiding this comment.
The I recommend that we use this "role" to mark sockets that are listening.
So we can drop the sock->local_port and sock->listen values as well.
It is not relevant to know in a boolean if you already called nrf_listen() or not, because nrf_accept() will fail if you did not.
There was a problem hiding this comment.
Given that we have no means of getting the ephemeral port from the modem, I think that it still makes sense to track whether we have bound to non-ephemeral port.
It is better to give an error than allow the user to do listen() to a port that they have no hope of getting.
There was a problem hiding this comment.
Actually, the role makes 0 sense as it is. No need to waste uint16_t for it when a bit field is enough.
Edit: That is, if we can just ignore the server parameter given in AT#XSOCKET and not return the same value when AT#XSOCKET? is done.
@trantanen: Does that constitute as breaking AT-interface?
-> Discussed offline, it does, so we keep it and return it in AT#XSOCKET?. Parameter does not have any effect internally.
There was a problem hiding this comment.
Also, AT#XLISTEN? currently returns the listen sockets and the ports that they are bound. Keep or remove?
-> Discussed offline: Keep.
There was a problem hiding this comment.
Actually, the role makes 0 sense as it is. No need to waste uint16_t for it when a bit field is enough.
Edit: That is, if we can just ignore the server parameter given in AT#XSOCKET and not return the same value when AT#XSOCKET? is done.
@trantanen: Does that constitute as breaking AT-interface?
As discussed, we won't change this. We keep compatibility no matter how useless it is.
207ba63 to
ac3bea8
Compare
Add AT#XLISTEN and AT#XACCEPT for TCP server functionality. Signed-off-by: Markus Lassila <markus.lassila@nordicsemi.no>
Basic unit tests for AT#XLISTEN and AT#XACCEPT. Signed-off-by: Markus Lassila <markus.lassila@nordicsemi.no>
ac3bea8 to
67ca2cf
Compare
Add AT#XLISTEN and AT#XACCEPT for TCP server functionality.
Jira: SM-240